-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
allow appending deployment key to username (#2062) #2169
allow appending deployment key to username (#2062) #2169
Conversation
fix (vcs.git): allow `+` in username
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a case where this still failed.
poetry/vcs/git.py
Outdated
@@ -9,7 +9,7 @@ | |||
|
|||
pattern_formats = { | |||
"protocol": r"\w+", | |||
"user": r"[a-zA-Z0-9_.-]+", | |||
"user": r"[a-zA-Z0-9_.\+-]+(:[\w\d]+)?", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a case in my usage where this doesn't appear to be enough. The deploy key for gitlab appears to allow -
characters, and the regex portion [\w\d]+
doesn't match when a -
is present.
Changing the [\w\d]+
to [\w\d-]+
got it working for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. Is there any source where it is described how the deployment key looks like? I wasn't able to find one :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't found any. The only suggestion I can make is to perhaps make the portion after the :
much less restrictive since it looks like the end of that match will be bounded by @
in all the patterns below.
I don't know what value there is in trying to restrict what can go after the :
unless there's a need for it to make the matching work properly.
hello, i have been running into the same problem. using the changes proposed in this PR solved the problem for me. can this get merged ? |
Is there something blocking this PR from being reviewed and/or merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLDR; we should parse for both user and password seperately. And possibly log a warning that this would mean credentials get stored as plain text value in both the lockfile and pyproject.toml
.
This will require rework.
When shall we expect the rework to be finished? |
@webyneter: @abn brings up a very good point I haven't care about when writing this PR: security. When we allow the schema So, we have to discuss whether we like to support this at all and if yes, if a warning about the security concerns are enough. fin swimmer |
Having this in poetry would make a lot of difference, for now I have to stick with 1.0.2 just to be able to install internal dependencies... |
The warning and log seems a reasonable approach for now. I would consider this a first pass. I wouldn't know how else people are storing their deploy tokens beyond it sitting in their requirements.txt file. Unless pip already has a way to do environment variable substitution into a requirement string? |
In the end, it falls to the project maintainers to decide how they want to go about it for their projects. Personally, having deployment keys in metadata and/or plain text git files are a no go. But ultimately, poetry is a tool and so we should allow valid configuration (ie. urls in the If the package is public, you are better off not needing a deployment key anyway /shrug. Eventually, there is a possibility that we can extend how poetry handles index credentials to also allow for repo credentials. However, there is still the issue of these being available in a distribution metadata. @webyneter is the use of |
@abn yes, exactly: I'm installing a python package from a private gitlab repo within my gitlab org, the project has got |
AFAIK the pip ecosystem doesn't have a nice way to hide this information either, and defaults to "use a private PYPI". If this workflow was to be supported and truly not store the credentials in metadata or files, I'm guessing it would be a lot of work. The thought I had about it would be the same way I would end up solving it for requirements.txt files: using jinja templating + environment variables to perform substitution at parse time. However, that doesn't necessarily ensure that the values don't get written back out into a lock file and other metadata files. Extra care would have to be taken to support that. But at the end of the day, pip lets you pass requirements formatted as such and install them so I personally don't think poetry should completely prevent it. I agree with @abn that it's not good practice but that it also shouldn't be entirely prevented. An extra option to force override and allow it seems like a reasonable middle ground between a full blown secure implementation and fully locking off the ability to use a pip feature. |
why check urls at all ? It's not like you can prevent using unreachable urls in the first place (server an go down etc), it can just introduce bugs. anyway, looking forward to a fix :) |
@teto: We need to parse the url to be able to identify the several parts like the source itself, the protocol used, the revision/tag/branch. The information must be stored in the correct format in the Anyway, the corresponding code has move to poetry-core. So I will move the PR over to it in the next days. |
Do the changes in this PR also address #2348? |
@finswimmer : Have you had a chance to create an equivalent PR for these changes on If not, what's the ETA on getting this merged? |
@setu4993 It's over here: python-poetry/poetry-core#96 But I plan to include some kind of warning as suggested by @abn in poetry itself. That's why it is a draft PR. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR allows appending deployment keys to usernames as used by gitlab. According to gitlab's docs a
+
is allowed in usernames. This is fixed as well.Fixes: #2062
Pull Request Check List
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!